geolocation: add GeoProbe state and CRUD instructions by nikw9944 · Pull Request #3120 · malbeclabs/doublezero

nikw9944 · 2026-02-27T15:31:50Z

Summary of Changes

Add GeoProbe account type to the Geolocation Program with CreateGeoProbe, UpdateGeoProbe, and DeleteGeoProbe instructions (foundation-gated via Serviceability CPI)
Create validates exchange is activated, IP is publicly routable, code length ≤32 bytes; delete enforces reference_count == 0
Add comprehensive IP validation that rejects all non-publicly-routable addresses (RFC 1918, loopback, multicast, link-local, reserved ranges, etc.)
Add GeoProbe PDA derivation (["doublezero", "probe", code.as_bytes()]) and extend serializer with try_acc_close for account deletion
Supports rfcs/rfc16-geolocation-verification.md

Diff Breakdown

Category	Lines
Prod code	~500
Test code	~1000

Testing Verification

Unit tests and integration tests

nikw9944 · 2026-02-27T17:07:26Z

smartcontract/programs/doublezero-geolocation/src/processors/geo_probe/create.rs

+        return Err(GeolocationError::InvalidServiceabilityProgramId.into());
+    }
+
+    // Verify it's a valid, activated Exchange account


FYI to reviewers: in RFC16 there's a to-do item to implement cross-program reference counting. We'll do it in a future PR to keep this PR more manageable.

Add GeoProbe account type with CreateGeoProbe, UpdateGeoProbe, and DeleteGeoProbe instructions. Includes IP validation (rejects all non-publicly-routable addresses), code length validation, exchange activation checks, and reference count protection on delete. Reorder GeoProbe struct fields to place variable-length fields (code, parent_devices) at the end for correct Borsh deserialization. Update RFC16 to match and remove latency_threshold_ns. Part 2 of 3 for RFC16 geolocation verification.

- Add comprehensive integration tests for create, update, and delete GeoProbe operations - Test validation logic for IP addresses, code length, and exchange status - Test reference count protection on deletion - Update CLAUDE.md to require integration tests for all processors - Remove unused system_program parameter from update processor

…me environment where the program upgrade authority check expects the payer to match the authority stored in the program data account

karl-dz

Left a few comments

karl-dz · 2026-02-27T22:08:08Z

smartcontract/programs/doublezero-geolocation/src/state/geo_probe.rs

+        if self.code.len() > 32 {
+            msg!("Code too long: {} bytes", self.code.len());
+            return Err(GeolocationError::InvalidCodeLength);
+        }


This would never be validated, would it? In the create instruction, you validate the code length, so I can't imagine a GeoProbe would ever have a code > 32.

Plus find_program_address would have borked if this were >32 anyway, so this feels impossible given these two reasons

karl-dz · 2026-02-27T22:10:23Z

smartcontract/programs/doublezero-geolocation/src/state/geo_probe.rs

+        if self.parent_devices.len() > MAX_PARENT_DEVICES {
+            msg!(
+                "Too many parent devices: {} (max {})",
+                self.parent_devices.len(),
+                MAX_PARENT_DEVICES
+            );
+            return Err(GeolocationError::MaxParentDevicesReached);
+        }


No instructions add parent devices yet. But I'm wondering if this would ever hit, too? I'm guessing when an instruction that introduces adding parent devices, there would be validation of whether this violates MAX_PARENT_DEVICES, so this condition would never be true.

Curious about a scenario where this would be true.

karl-dz · 2026-02-27T22:11:33Z

smartcontract/programs/doublezero-geolocation/src/state/geo_probe.rs

+impl Validate for GeoProbe {
+    fn validate(&self) -> Result<(), GeolocationError> {
+        if self.account_type != AccountType::GeoProbe {
+            msg!("Invalid account type: {}", self.account_type);
+            return Err(GeolocationError::InvalidAccountType);
+        }
+        if self.code.len() > 32 {
+            msg!("Code too long: {} bytes", self.code.len());
+            return Err(GeolocationError::InvalidCodeLength);
+        }
+        if self.parent_devices.len() > MAX_PARENT_DEVICES {
+            msg!(
+                "Too many parent devices: {} (max {})",
+                self.parent_devices.len(),
+                MAX_PARENT_DEVICES
+            );
+            return Err(GeolocationError::MaxParentDevicesReached);
+        }
+        Ok(())
+    }
+}


Now that I've written the two comments above, I'm wondering why this trait even needs to be implemented with any of the accounts you have in this smart contract. What does calling validate protect against exactly?

karl-dz · 2026-02-27T22:14:07Z

smartcontract/programs/doublezero-geolocation/src/pda.rs

+    #[test]
+    fn test_program_config_pda_is_deterministic() {
+        let program_id = test_program_id();
+        let (pda1, bump1) = get_program_config_pda(&program_id);
+        let (pda2, bump2) = get_program_config_pda(&program_id);
+        assert_eq!(pda1, pda2);
+        assert_eq!(bump1, bump2);
+    }
+
+    #[test]
+    fn test_program_config_pda_differs_by_program_id() {
+        let (pda1, _) = get_program_config_pda(&Pubkey::new_unique());
+        let (pda2, _) = get_program_config_pda(&Pubkey::new_unique());
+        assert_ne!(pda1, pda2);
+    }
+
+    #[test]
+    fn test_geo_probe_pda_is_deterministic() {
+        let program_id = test_program_id();
+        let (pda1, bump1) = get_geo_probe_pda(&program_id, "probe-a");
+        let (pda2, bump2) = get_geo_probe_pda(&program_id, "probe-a");
+        assert_eq!(pda1, pda2);
+        assert_eq!(bump1, bump2);
+    }
+
+    #[test]
+    fn test_geo_probe_pda_differs_by_code() {
+        let program_id = test_program_id();
+        let (pda1, _) = get_geo_probe_pda(&program_id, "probe-a");
+        let (pda2, _) = get_geo_probe_pda(&program_id, "probe-b");
+        assert_ne!(pda1, pda2);
+    }
+
+    #[test]
+    fn test_geo_probe_pda_differs_by_program_id() {
+        let (pda1, _) = get_geo_probe_pda(&Pubkey::new_unique(), "probe-a");
+        let (pda2, _) = get_geo_probe_pda(&Pubkey::new_unique(), "probe-a");
+        assert_ne!(pda1, pda2);
+    }


These tests don't seem necessary (and would argue the one below this comment isn't either, too). What do you think?

karl-dz · 2026-02-27T22:15:52Z

smartcontract/programs/doublezero-geolocation/tests/geo_probe_test.rs

+    let probe = GeoProbe::try_from(&probe_account.data[..]).unwrap();
+
+    assert_eq!(probe.account_type, AccountType::GeoProbe);
+    assert_eq!(probe.owner, payer.pubkey());
+    assert_eq!(probe.exchange_pk, exchange_pubkey);
+    assert_eq!(probe.public_ip, Ipv4Addr::new(8, 8, 8, 8));
+    assert_eq!(probe.location_offset_port, 4242);
+    assert_eq!(probe.metrics_publisher_pk, args.metrics_publisher_pk);
+    assert_eq!(probe.reference_count, 0);
+    assert_eq!(probe.code, code);
+    assert_eq!(probe.parent_devices.len(), 0);


FYI, if GeoProbe derives PartialEq then it would be better to assert_eq!(probe, expected_probe) in case you add more fields to your geo probe account in the future. With the way this is written, you run the risk of missing asserting that the new fields will be equal what you expect

karl-dz · 2026-02-27T22:16:52Z

smartcontract/programs/doublezero-geolocation/tests/geo_probe_test.rs

+    );
+
+    let result = banks_client.process_transaction(tx).await;
+    assert!(result.is_err());


Does not seem necessary since you unwrap the error later on. Should the claude markdown be informed to not perform this is_err check?

karl-dz · 2026-02-27T22:17:16Z

smartcontract/programs/doublezero-geolocation/tests/geo_probe_test.rs

+    assert!(result.is_err());
+    // Exchange not activated should return InvalidAccountData


Do you want to assert that the error equals something specific?

karl-dz · 2026-02-27T22:18:26Z

smartcontract/programs/doublezero-geolocation/tests/geo_probe_test.rs

+    // Verify immutable fields didn't change
+    assert_eq!(probe.code, code);
+    assert_eq!(probe.exchange_pk, exchange_pubkey);


I like the assertion that immutable fields don't change. But like the comment above, might be better to assert full struct equality in case more immutable fields get added

karl-dz · 2026-02-27T22:20:08Z

smartcontract/programs/CLAUDE.md

+
+1. **Assert specific errors**: Tests should assert specific error types (e.g., `ProgramError::Custom(17)`), not just `.is_err()`. This catches regressions where the instruction fails for the wrong reason.
+
+2. **Don't test framework functionality**: Avoid writing tests that only exercise SDK/framework behavior (e.g., testing that `Pubkey::find_program_address` is deterministic or produces different outputs for different inputs). Focus tests on your program's logic.


I guess when the unit tests were generated in this PR, claude forgot about this

karl-dz · 2026-02-27T22:20:30Z

smartcontract/programs/CLAUDE.md


+### Testing
+
+1. **Assert specific errors**: Tests should assert specific error types (e.g., `ProgramError::Custom(17)`), not just `.is_err()`. This catches regressions where the instruction fails for the wrong reason.


nikw9944 linked an issue Feb 27, 2026 that may be closed by this pull request

geolocation: smartcontract program part 2 - geoprobe #3122

Open

nikw9944 commented Feb 27, 2026

View reviewed changes

ben-dz self-requested a review February 27, 2026 18:51

nikw9944 force-pushed the bdz/doublezero-2952-part-2-geo-probe-crud branch from 27afe8a to df46177 Compare February 27, 2026 19:52

nikw9944 added 2 commits February 27, 2026 20:59

nikw9944 force-pushed the bdz/doublezero-2952-part-2-geo-probe-crud branch from df46177 to 1dfbf75 Compare February 27, 2026 20:59

geolocation: test infrastructure now properly mimics the Solana runti…

083f58f

…me environment where the program upgrade authority check expects the payer to match the authority stored in the program data account

nikw9944 force-pushed the bdz/doublezero-2952-part-2-geo-probe-crud branch from 1dfbf75 to 083f58f Compare February 27, 2026 21:03

karl-dz requested changes Feb 27, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

geolocation: add GeoProbe state and CRUD instructions#3120

geolocation: add GeoProbe state and CRUD instructions#3120
nikw9944 wants to merge 3 commits intomainfrom
bdz/doublezero-2952-part-2-geo-probe-crud

nikw9944 commented Feb 27, 2026 •

edited

Loading

Uh oh!

nikw9944 Feb 27, 2026 •

edited

Loading

Uh oh!

karl-dz left a comment

Uh oh!

karl-dz Feb 27, 2026

Uh oh!

karl-dz Feb 27, 2026

Uh oh!

karl-dz Feb 27, 2026

Uh oh!

karl-dz Feb 27, 2026

Uh oh!

karl-dz Feb 27, 2026

Uh oh!

karl-dz Feb 27, 2026

Uh oh!

karl-dz Feb 27, 2026

Uh oh!

karl-dz Feb 27, 2026

Uh oh!

karl-dz Feb 27, 2026

Uh oh!

karl-dz Feb 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		assert!(result.is_err());
		// Exchange not activated should return InvalidAccountData


		1. Assert specific errors: Tests should assert specific error types (e.g., `ProgramError::Custom(17)`), not just `.is_err()`. This catches regressions where the instruction fails for the wrong reason.

		2. Don't test framework functionality: Avoid writing tests that only exercise SDK/framework behavior (e.g., testing that `Pubkey::find_program_address` is deterministic or produces different outputs for different inputs). Focus tests on your program's logic.


		### Testing

		1. Assert specific errors: Tests should assert specific error types (e.g., `ProgramError::Custom(17)`), not just `.is_err()`. This catches regressions where the instruction fails for the wrong reason.

Conversation

nikw9944 commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Diff Breakdown

Testing Verification

Uh oh!

nikw9944 Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karl-dz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nikw9944 commented Feb 27, 2026 •

edited

Loading

nikw9944 Feb 27, 2026 •

edited

Loading